feat(workflow): H2→preview server auto-launch + /pf:preview slash command (Phase 2)#104
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
개요이 PR은 새로운 변경 사항
시퀀스 다이어그램sequenceDiagram
participant CLI as CLI/사용자
participant Script as start-preview-server.sh
participant Detect as 환경 감지
participant Setup as 설치/실행
participant Server as 웹 서버
participant Browser as 브라우저
CLI->>Script: /pf:preview start 실행
Script->>Detect: docker-compose.yml 또는<br/>apps/api/web 감지
alt Docker 프로필
Detect-->>Setup: docker compose up -d
Setup->>Server: Docker 컨테이너 시작
else 표준 프로필
Detect-->>Setup: pnpm/npm install
Setup->>Server: API/Web dev 프로세스 생성
end
Setup->>Setup: 자유 포트 프로빙 & TCP 대기
Setup->>Script: 서버 준비 완료
Script->>Browser: localhost URL 열기
Browser-->>CLI: 미리보기 서버 활성화
예상 코드 리뷰 노력🎯 4 (복잡함) | ⏱️ ~45분 관련 가능성이 있는 PR
시
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98d940e201
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ -f "$run_dir/docker-compose.yml" ]; then | ||
| profile="docker" | ||
| elif [ -f "$run_dir/apps/api/package.json" ] && [ -f "$run_dir/apps/web/package.json" ]; then | ||
| profile="standard" |
There was a problem hiding this comment.
The new start path aborts with exit 2 unless docker-compose.yml or apps/{api,web}/package.json exists directly under run_dir, but the project’s own engineering workflow writes runnable app artifacts under runs/<id>/generated/ (for example plugins/preview-forge/agents/engineering/backend/be-lead.md uses cd runs/<id>/generated && pnpm -r build). Because /pf:freeze and /pf:preview now instruct callers to pass runs/<id>/, valid frozen runs can be misclassified as “missing scaffold” and never auto-launch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
지적해주신 P1 정확합니다. be-lead.md / QA leads는 runs/<id>/generated/ 에 산출물을 쓰는데, /pf:freeze · /pf:preview 는 runs/<id>/ 를 넘기고 있어서 detect-fail로 exit 2가 나왔습니다.
5a8b3e6에서 profile detection이 <run_dir> 와 <run_dir>/generated/ 를 순서대로 probe 하도록 수정했고, scaffold_root 변수로 통일해서 docker_project_up · stop · pnpm install · api_dir/web_dir 모두 같은 경로를 따르게 했습니다. dry-run으로 두 케이스(direct/generated × standard/docker) 회귀 확인했습니다.
| for p in rec.get("Publishers") or []: | ||
| pub = p.get("PublishedPort") |
There was a problem hiding this comment.
Docker’s docker compose ps reference says --format json outputs container info “as a JSON array”; in this parser, rec = json.loads(line) is followed by rec.get(...), which raises when rec is a list. That exception is swallowed by || true, leaving port empty and forcing the fallback 18080, so environments where Compose publishes a different host port will open the wrong preview URL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
P2 valid. coderabbit의 같은 라인 지적과 동일하게, rec = json.loads(line) 후 rec.get(...)은 rec이 list일 때 AttributeError로 떨어졌고, 그게 || true로 묻혀 fallback 18080으로 흘렀습니다.
5a8b3e6에서 stdin 전체를 한 번에 읽고 (a) json.loads(raw)로 array/object 시도 → (b) 실패 시 NDJSON 분기로 fallback 하는 형태로 단순화했습니다. isinstance(rec, dict) 가드도 추가해서 list 원소 자체가 dict이 아닐 때도 skip 하도록 했습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-plugin.sh (1)
8-8:⚠️ Potential issue | 🟡 Minor헤더 체크리스트 주석 갱신 누락 (
14 → 15).Line 78/80에서 슬래시 커맨드 기대 개수를 15로 올렸지만, 파일 상단 체크 요약(Line 8)은 여전히 "14 slash commands present"입니다. 문서 일관성 차원에서 함께 업데이트하면 좋겠습니다.
📝 Proposed fix
-# 3. 14 slash commands present +# 3. 15 slash commands present🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-plugin.sh` at line 8, Update the header checklist comment in scripts/verify-plugin.sh so it matches the adjusted expected slash command count: change the line that currently reads "3. 14 slash commands present" to reflect "15 slash commands present" (this is the top-of-file checklist comment referenced in the diff and relates to the verify logic that was updated at lines ~78/80).
🧹 Nitpick comments (1)
scripts/start-preview-server.sh (1)
378-384: 정의만 되어 있고 호출되지 않는spawn함수.Line 378-381의
spawn()은 어디에서도 호출되지 않고, 실제 spawn은 Line 383-384에서 인라인으로 처리됩니다.eval기반 함수가 굳이 필요하지 않다면 정의를 제거하거나, 추출하려던 의도가 맞다면 두 호출부를spawn호출로 바꿔 주세요(중복 제거 +extra_env분기 일원화).♻️ Option A — remove dead helper
-# Spawn api + web in background, redirecting output. setsid (if available) -# detaches them from the controlling tty so they survive shell exit. -spawn() { - local dir="$1" port="$2" log="$3" extra_env="$4" - ( cd "$dir" && eval "$extra_env PORT=$port nohup $dev_cmd >'$log' 2>&1 &" echo $! ) -} - api_pid="$( ( cd "$api_dir" && PORT="$api_port" nohup $dev_cmd >"$API_LOG" 2>&1 & echo $! ) )" web_pid="$( ( cd "$web_dir" && PORT="$web_port" NEXT_PUBLIC_API_URL="http://localhost:$api_port" nohup $dev_cmd >"$WEB_LOG" 2>&1 & echo $! ) )"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start-preview-server.sh` around lines 378 - 384, The spawn() helper is defined but unused; either remove it to eliminate dead code or replace the inline process starts that set api_pid and web_pid with calls to spawn to centralize env handling. If you choose to use it, update the two invocations that currently inline nohup (the blocks assigning api_pid and web_pid) to call spawn with dir vars (api_dir/web_dir), port (api_port/web_port), log files (API_LOG/WEB_LOG) and appropriate extra_env (for web add NEXT_PUBLIC_API_URL="http://localhost:$api_port"); ensure spawn uses dev_cmd and echoes the PID as before. If you remove it, delete the spawn() function and keep the inline starts unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/preview-forge/agents/meta/chief-engineer-pm.md`:
- Around line 107-121: The H2 auto-launch text declares M3 will run bash
scripts/start-preview-server.sh (stateful) which conflicts with the document's
allowed_scope Bash whitelist and Rule 6; fix by changing the H2 auto-launch
behavior to delegate execution to a Write-capable sub-agent instead of M3
invoking start-preview-server.sh directly (update the H2 block to describe a
sub-agent trigger and payload), and keep allowed_scope unchanged (or
alternatively if you choose the whitelist route, explicitly add
start-preview-server.sh to allowed_scope with a short justification why it is a
sanctioned exception). Ensure references to M3, start-preview-server.sh,
allowed_scope, Rule 6, and the H2 auto-launch block are updated consistently.
In `@scripts/start-preview-server.sh`:
- Around line 283-310: The current python one-liner in the docker port
extraction block re-parses the same line on JSONDecodeError and never correctly
handles a single JSON array vs NDJSON; replace the per-line parsing in the
python3 -c script used inside the port="$(... | python3 -c '...')" call with
logic that reads all stdin at once, tries a single json.loads on the whole input
to detect a JSON array (iterate its elements and their "Publishers"), and if
that fails, fallback to splitting stdin into lines and parsing each line as
NDJSON, printing the first found PublishedPort; update the script inside the
python3 -c invocation (the small script in the diff) accordingly so it correctly
handles both formats and exits after printing the port.
- Around line 383-398: The script writes api_pid and web_pid to PID_FILE but
exits on wait_tcp timeout without killing those background processes, causing
leaked pids; update the startup to register a cleanup trap (or explicit error
path) that kills $api_pid and $web_pid (and removes PID_FILE) when wait_tcp
fails or the script exits early: add a trap handler function (e.g., cleanup or
on_exit) and install it via trap 'cleanup' EXIT (or specifically on ERR) before
launching processes, ensure the handler checks for non-empty api_pid/web_pid and
uses kill (with fallback to kill -9) and then removes PID_FILE and any partial
URL_FILE so retries don't reuse stale processes; reference variables api_pid,
web_pid, PID_FILE, URL_FILE and the wait_tcp failure branch to locate where to
add the trap and cleanup logic.
---
Outside diff comments:
In `@scripts/verify-plugin.sh`:
- Line 8: Update the header checklist comment in scripts/verify-plugin.sh so it
matches the adjusted expected slash command count: change the line that
currently reads "3. 14 slash commands present" to reflect "15 slash commands
present" (this is the top-of-file checklist comment referenced in the diff and
relates to the verify logic that was updated at lines ~78/80).
---
Nitpick comments:
In `@scripts/start-preview-server.sh`:
- Around line 378-384: The spawn() helper is defined but unused; either remove
it to eliminate dead code or replace the inline process starts that set api_pid
and web_pid with calls to spawn to centralize env handling. If you choose to use
it, update the two invocations that currently inline nohup (the blocks assigning
api_pid and web_pid) to call spawn with dir vars (api_dir/web_dir), port
(api_port/web_port), log files (API_LOG/WEB_LOG) and appropriate extra_env (for
web add NEXT_PUBLIC_API_URL="http://localhost:$api_port"); ensure spawn uses
dev_cmd and echoes the PID as before. If you remove it, delete the spawn()
function and keep the inline starts unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36ee313c-56da-417b-a116-5a8131d0852a
📒 Files selected for processing (5)
plugins/preview-forge/agents/meta/chief-engineer-pm.mdplugins/preview-forge/commands/freeze.mdplugins/preview-forge/commands/preview.mdscripts/start-preview-server.shscripts/verify-plugin.sh
- Detect scaffold under <run_dir>/generated/ in addition to <run_dir>/ so freshly-frozen runs (be-lead writes apps to runs/<id>/generated/) auto-launch instead of mis-aborting with exit 2 (codex P1). - Replace per-line `docker compose ps --format json` parser with read-all-stdin logic that correctly handles both NDJSON and single JSON-array formats (codex P2 + coderabbit minor). - Add cleanup_spawned trap so wait_tcp 60s timeout no longer leaks api/web pids; PID_FILE is removed and SIGTERM sent to both children before exit (coderabbit major). - Remove unused spawn() helper to eliminate dead code (coderabbit nitpick). - chief-engineer-pm.md: explicitly whitelist start-preview-server.sh in allowed_scope as a v1.7.0+ Phase 2 sanctioned exception, resolving the Rule 6 conflict between the H2 auto-launch block and the read-only Bash policy (coderabbit major). - verify-plugin.sh: sync header checklist comment 14 -> 15 to match the command-count assertion at line 80 (coderabbit minor, outside-diff). Refs PR #104 review comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… launcher (Phase 2)
Bridges H2 freeze → live local app gap (DEMO-STORYBOARD.md L1:50–2:00).
Auto-detects profile (docker-compose.yml vs apps/{api,web}/package.json),
picks free port from 18080+, spawns dev servers in background, opens browser.
Idempotent (re-runs only re-open URL), supports stop + status subcommands.
PF_PREVIEW_DRY_RUN=1 keeps unit tests light.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User-facing manual entry point for re-opening or stopping the local preview server post-H2 (e.g. after a reboot or explicit /pf:preview stop). Wraps scripts/start-preview-server.sh; defaults to most-recent run when run_id omitted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ase 2) Adds an HTML-comment-delimited block to chief-engineer-pm.md instructing M3 to invoke scripts/start-preview-server.sh immediately after H2 approval — matching the markdown-imperatives pattern Phase 1 uses for H1→SpecDD. freeze.md gains a footer note describing the same auto-launch and the manual /pf:preview re-open / stop commands. The HTML delimiter keeps the H2 block textually distinct from any §3.9 H1 dispatch block Phase 1 may add, so the two PRs merge cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Detect scaffold under <run_dir>/generated/ in addition to <run_dir>/ so freshly-frozen runs (be-lead writes apps to runs/<id>/generated/) auto-launch instead of mis-aborting with exit 2 (codex P1). - Replace per-line `docker compose ps --format json` parser with read-all-stdin logic that correctly handles both NDJSON and single JSON-array formats (codex P2 + coderabbit minor). - Add cleanup_spawned trap so wait_tcp 60s timeout no longer leaks api/web pids; PID_FILE is removed and SIGTERM sent to both children before exit (coderabbit major). - Remove unused spawn() helper to eliminate dead code (coderabbit nitpick). - chief-engineer-pm.md: explicitly whitelist start-preview-server.sh in allowed_scope as a v1.7.0+ Phase 2 sanctioned exception, resolving the Rule 6 conflict between the H2 auto-launch block and the read-only Bash policy (coderabbit major). - verify-plugin.sh: sync header checklist comment 14 -> 15 to match the command-count assertion at line 80 (coderabbit minor, outside-diff). Refs PR #104 review comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sgwannabe
force-pushed
the
feat/h2-preview-server-autolaunch
branch
from
5a8b3e6 to
b914de0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b914de0a1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "start-preview-server.sh: docker not on PATH but $compose_file requires it" >&2 | ||
| exit 1 | ||
| fi | ||
| docker compose -f "$compose_file" up -d --quiet-pull >/dev/null 2>&1 || { |
There was a problem hiding this comment.
This docker compose ... up call omits -p, so when the compose file is under <run_dir>/generated/docker-compose.yml every run resolves to the same default project name (generated). Docker’s compose reference says that without -p, project name falls back to the basename of the directory containing the config file, which means different run IDs can overwrite each other’s stack and status/stop may operate on the wrong run. Pass a per-run project name consistently on all compose calls (up, ps, and down).
Useful? React with 👍 / 👎.
| if kill -0 "$pid" 2>/dev/null; then | ||
| echo "$pid" | ||
| any=0 | ||
| fi |
There was a problem hiding this comment.
pids_alive marks the preview as running when any PID in .preview-server.pid is alive, but standard mode launches two processes (api and web). If one process crashes and the other survives, start/status still treat the server as healthy, causing idempotent re-open instead of restart and leaving the preview URL broken. The health check should verify both role PIDs (or at least confirm the web listener) before returning success.
Useful? React with 👍 / 👎.
1 participant
Summary
Phase 2 of 2 (companion: PR #103 for H1→SpecDD). Closes user-reported gap: after H2 freeze, no local preview server starts; user has to
pnpm -r devordocker compose upmanually, breaking DEMO-STORYBOARD.md L1:50–2:00 (which promiseslocalhost:18080opens in a new tab automatically).scripts/start-preview-server.sh(~360 LOC including comments) — profile-aware launcher (docker-compose for pro/max, apps/{api,web} pnpm dev for standard, exit-2 for missing scaffold). Free-port scan from 18080+, idempotent re-open, stop/status subcommands,PF_PREVIEW_DRY_RUN=1for CI./pf:preview [run_id]slash command (auto-discovered; commands count 14→15 reflected inverify-plugin.sh).chief-engineer-pm.mdH2 section gains an HTML-comment-delimited block making the auto-launch imperative explicit (markdown-imperatives pattern matching Phase 1).freeze.mdfooter note reflects the auto-launch + manual/pf:preview <id>re-open path.Side-effect / breaking-change table
/pf:freezesucceeds. User no longer needs to run dev servers manually. Documented infreeze.mdfooter + M3 H2 imperative.pick_free_portscans 18080..18090 (web) + 18180..18190 (api) vialsof -iTCP:<p> -sTCP:LISTEN; bash/dev/tcpfallback iflsofabsent. Exits 1 with stderr if all 11 are taken.lsof,pnpm/npm,docker,ncall standard or feature-detected.setsidnot used;nohup &for background detachment is portable. Windows out of scope (per plan).startinvocation checks.preview-server.pid(PIDs alive viakill -0) or docker project state; on hit, only re-opens URL — no double-spawn. Test: dry-run profile detection on existing fixture.dispatch-spec-cycle.sh,post-h1-signal.py,hooks/hooks.json,commands/new.md,agents/meta/run-supervisor.md. The Phase 2 H2 block is HTML-comment-delimited (<!-- H2→preview-server auto-launch -->) and lives between line 105 (<!-- end A-5 -->) and line 107 (### 4.), separate from any Phase 1 §3.9 H1 block.Verification
Unit (mock fixtures):
statuson no-server → exit 1: PASSstopon no-server → exit 0 (idempotent): PASSPF_PREVIEW_DRY_RUN=1standard profile (mockapps/api/package.json+apps/web/package.json): prints would-actions, exit 0: PASSPF_PREVIEW_DRY_RUN=1docker profile (mockdocker-compose.yml): prints would-actions, exit 0: PASSRegression suites (all green):
scripts/verify-plugin.sh— 58/0 (commands count updated 14→15,/pf:previewregistered)tests/fixtures/security/verify-security.shtests/test-advocate-boilerplate.shtests/fixtures/h1-modal-swap/verify.shtests/fixtures/lesson07-regression/verify-lesson07.shtests/fixtures/spec-anchor-convergence/verify.shtests/fixtures/filled-ratio-gating/verify.shtests/fixtures/cache-concurrency/test-{race-window,self-heal,5way}.shtests/e2e/mock-bootstrap.sh {standard,pro,max}— 9/18/26 iframes (expected)Codex review
Local
codex review --base origin/mainblocked by CLI version mismatch on this workstation: codex-cli 0.121.0 default modelgpt-5.5reports "requires a newer version of Codex"; account-supported alternatives (gpt-5,gpt-5-codex,o4-mini,o3,gpt-4o) all returnnot supported when using Codex with a ChatGPT account. Codex P1/P2/P3 counts: 0/0/0 (review unrun). Recommend a follow-up codex pass after CLI upgrade or a maintainer-side review.Test plan
chief-engineer-pm.mdruns/r-*-standard/workspace,bash scripts/start-preview-server.sh runs/<id>/opens browser tohttp://localhost:18080/bash scripts/start-preview-server.sh stop runs/<id>/cleanly terminates (verify withlsof -iTCP:18080)startafterstopworks fresh (no stale.preview-server.pidfailure)pro/maxworkspace withdocker-compose.yml,startrunsdocker compose up -dand opens the first published port🤖 Generated with Claude Code
Summary by CodeRabbit
릴리스 노트
새로운 기능
문서